Start documenting error handling in coding guidelines#50
Conversation
---- Cherry-picked commit 2711584 from Francesco's coding-guidelines-error-reporting branch (PR 48)
---- Applied changes suggested by Amos for Francesco's coding-guidelines-error-reporting branch (PR 48).
Inspired by github.com/measurement-factory/squid-notes/blob/start/errors.md
|
Marking as a draft because I do not know how to test how this markup is going to be rendered on the web site. |
|
Several key decisions in these proposed guidelines were driven by unofficial notes at https://github.com/measurement-factory/squid-notes/blob/start/errors.md |
|
LGTM in level of detail and to drive AI; it feels a bit verbose to include in the main coding guidelines. Would it be sensible to move it to its own page? |
Yes, I think it would, with a couple of bullet points remaining in SquidCodingGuidelines, so that folks reading our guidelines can discover the essential requirements (and then find the details on a dedicated page). I will adjust this PR to implement your suggestion. |
Co-authored-by: Francesco Chemolli <kinkie@squid-cache.org>
rousskov
changed the title
Done in 235f7b2. Still untested. |
| allowed, but Squid currently avoids explicit `constexpr`, and sprinkling | ||
| Squid code with many `constexpr` specifiers to get some compile-time | ||
| assertion working is usually a bad idea. |
There was a problem hiding this comment.
"avoids" implies that we have some sort of agreement not to use that construct. Reality is that no Squid contributor has considered it worth converting any of the code to using it.
I suggest leaving it at: (no more than) minor adjustments being acceptable.
| allowed, but Squid currently avoids explicit `constexpr`, and sprinkling | |
| Squid code with many `constexpr` specifiers to get some compile-time | |
| assertion working is usually a bad idea. | |
| allowed, but these should not extend nor exceed the minimal scope of logic change. |
| sent a syntactically valid HTTP request to Squid"), do not use any of the | ||
| above calls. Instead, create and throw a `TextException` object, return |
There was a problem hiding this comment.
"if X, do not use any of the above" - directly contradicts the initial requirement "pick the first one that matches your use case".
This whole clause/condition is not clearly distinguished from the prior "code invariant" clause. Likely because this item is about transactional / protocol flow behaviour; whereas the prior clauses are about catching code logic errors.
| request is actually dedicated to upgrading legacy code. In those exceptional | ||
| cases, the author becomes responsible for providing a high quality | ||
| replacement, of course. |
There was a problem hiding this comment.
| request is actually dedicated to upgrading legacy code. In those exceptional | |
| cases, the author becomes responsible for providing a high quality | |
| replacement, of course. | |
| request is actually dedicated to upgrading legacy code. |
Code quality is subjective and your definition has proven to be both controversial and problematic. Do not make it a requirement for Squid contributors to meet that impossible bar. Remember that we have the officially core/board agreed policy that "good enough" is permissible - "high quality" is an Alex review thing, not a Squid code requirement.
| ## Other special cases | ||
|
|
||
| `Assure()` is not available in code residing outside of `src/`, in helper code | ||
| that has not been upgraded to use `src/base` APIs, and in legacy C code. Use |
There was a problem hiding this comment.
| that has not been upgraded to use `src/base` APIs, and in legacy C code. Use | |
| that has not been upgraded to use `src/base` APIs, and in legacy C-style callbacks. Use |
The only actual "legacy C code" is outside src/ now, so strictly speaking that would be redundant text. However I suspect you actually mean the legacy code in src/ which is using the old C-style mechanisms. Of which the most notable are callbacks. If there are others, it might be worth enumerating theme here and also as items to fix in the contributor TODO list.
| * Legacy `TexcHere()` is a convenience macro. Modern code spells out | ||
| `TextException` and `Here()` explicitly. | ||
|
|
||
| Keep in mind that it is usually best to leave legacy code intact. Upgrading |
There was a problem hiding this comment.
Keep in mind that it is usually best to leave legacy code intact.
Hmm, this statement implies that you have seriously misunderstood the Best Practice principle of leaving old code alone. The purpose of which is actually to prevent young/new developers a) unnecessarily introducing bugs with code change, b) removing necessary bug workarounds while cleaning "messy" legacy code, or c) re-re-upgrading the same code to "latest new thing" in a loop instead of fixing underlying problems.
The practice only holds true when a) the developer is ignorant of the old code purpose and issues within, or b) the replacement "latest thing" is not yet proven to be better code.
The Squid code has reached the pace where almost all code is now outdated "legacy" patched together by layers of technical debt (eg wrapper macros and functions). Leaving the technical debt intact has long since stopped being a good thing.
| The following error handling functions are not covered by this documentation. | ||
| They should be avoided in most cases, especially in new code: `fatalf()`, | ||
| `fatal()`, `fatal_dump()`, `xassert()`. |
There was a problem hiding this comment.
These are the same paradigm as the Must() etc, items you put in "Legacy error handling". Please either
Either;
- document all of the mechanisms currently existing (even deprecated ones), or
- omit the legacy/deprecated items entirely (proposed text about them in the
SquidCodingGuidelinespage is sufficient).
The author should exclusively use whatever of the mechanisms documented on this page are appropriate for their new/changed code.
| * Do not use deprecated `Must()`, `Must3()`, fatalf()`, `fatal()`, and | ||
| `fatal_dump()` in new code. See |
There was a problem hiding this comment.
Missing xassert() and TexcHere()
3 participants
No description provided.